Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

undefinedとnullの厳密比較を禁止する設定を追加 #1585

Merged
merged 3 commits into from
Sep 27, 2023

Conversation

Tksn07
Copy link
Contributor

@Tksn07 Tksn07 commented Sep 25, 2023

内容

↓ こちらのissueを修正したPRになります。

eslintにundefinedとnullの厳密比較を禁止する設定を追加しました。

下記のコードを、main.tsなどに張り付けていただければ確認できます。

const fuga = "null";
const a = fuga != undefined; // OK
const a_1 = fuga !== undefined; // NG

const b = fuga == undefined; // OK
const b_1 = fuga === undefined; // NG

const c = fuga != null; // OK
const c_2 = fuga !== null; // NG

const d = fuga == null; // OK
const d_2 = fuga === null; // NG

const e = "fuga" == undefined; // OK
const e_2 = "fuga" === undefined; // NG

const button = { text: null };

const f = button.text == null; // OK
const f_2 = button.text === null; // NG

const g = "fuga" == null; // OK
const g_2 = "fuga" === null; // NG

関連 Issue

ref #1513

スクリーンショット・動画など

image

その他

初めてOSSにPRを切らせていただきました。
何かやり方が間違っているなどあれば教えていただけると幸いです🙇

@Tksn07 Tksn07 requested a review from a team as a code owner September 25, 2023 16:03
@Tksn07 Tksn07 requested review from y-chan and removed request for a team September 25, 2023 16:03
.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
@Tksn07
Copy link
Contributor Author

Tksn07 commented Sep 26, 2023

@sevenc-nanashi こちらのeslintを追加する場合、現段階で、 === null=== undefined を使っている箇所が沢山あり、エラーが大量に起きてしまいますが、こちらは別PRを切って対処する形でよろしいでしょうか?👀

(修正範囲が大きすぎるので細かく対処していった方が良さそうと考えてます。。。)

@sevenc-nanashi
Copy link
Member

sevenc-nanashi commented Sep 26, 2023

うーん、どうなんでしょう? @Hiroshiba

個人的にはmainのCIは全部通るようにしておきたい+最悪cherry-pickした別ブランチからPRしなおせばいいのでこのPRでやっちゃってもいいと思いますが、ヒホさん(Hiroshibaさん)の判断優先でお願いします。

(ちなみに、suggestionの「Commit Suggestion」を押すといい感じにやってくれます、レビューの解決とかCo-Author登録とか)

@Hiroshiba
Copy link
Member

Hiroshiba commented Sep 26, 2023

プルリクエストありがとうございます!!!

エラー量見てみました、確かに凄まじい数ですね・・・!
ESlintで出ているエラーをinfoあるいは警告にして、mainブランチにマージするというのはどうでしょう?

@Hiroshiba Hiroshiba requested review from Hiroshiba and removed request for y-chan September 26, 2023 16:47
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちょっと別のコメントなんですが、nullやundefinedと比較していなさそうなのにエラーになっている例がかなりあるかもしれません。

例えばこちらとか・・・。
https://github.com/VOICEVOX/voicevox/pull/1585/files#file-src-background-ts-L361
const COMBINATION_IS_NONE = "####";です)

これ、なんか直感的にかなり難しい問題な気がしました 😇

@Tksn07
Copy link
Contributor Author

Tksn07 commented Sep 27, 2023

@Hiroshiba @sevenc-nanashi 見てくださりありがとうございます🙇‍♂️
ESLintのerrorをinfoかwarningにするのは良いと思います!
ESLintのエラー自体は{ off, warning, error } の3つなので、warning を採用する形で進めようと思います🙌
https://eslint.org/docs/latest/use/configure/rules#rule-severities

また、こちらに関してですが、

ちょっと別のコメントなんですが、nullやundefinedと比較していなさそうなのにエラーになっている例がかなりあるかもしれません。

例えばこちらとか・・・。 https://github.com/VOICEVOX/voicevox/pull/1585/files#file-src-background-ts-L361const COMBINATION_IS_NONE = "####";です)

これ、なんか直感的にかなり難しい問題な気がしました 😇

自分の方で調べてみてもなぜこれがエラーになるのかの原因を特定することはできませんでした😇
(恐らくESLint側の内部的なバグと思われる)

その対処法として今回のissueの対応では、
=== null, === undefined などの厳密比較に対して右辺にundefined, null 持って来るケースに対応したい。という内容なので、

(変更前)selector: "BinaryExpression[operator='==='][right.value=undefined]",
(変更後)selector: "BinaryExpression[operator='==='][right.name=undefined]",

といった、right.value -> right.name にして対応する方針でどうでしょうか?

nameになるので、

const undefined = "test"
const null = "test"

などもエラーになりますが、予約語であり別の制約に引っかかるのでnameにしても問題なさそうと考えています👀

@Hiroshiba
Copy link
Member

ESLintのエラー自体は{ off, warning, error } の3つなので、warning を採用する形で進めようと思います🙌
eslint.org/docs/latest/use/configure/rules#rule-severities

おお、なるほどです!!良さそうに感じました!

@Hiroshiba
Copy link
Member

といった、right.value -> right.name にして対応する方針でどうでしょうか?

これで試してみたのですが、まだ誤検知があるようでした。
ちょっと気になったのでいろいろ試してみて、これでどうだろうというのを考えてみたので別でコメントします!

便利なあれこれ見つけたのでメモです。

このツールが便利でした。 esquery

検出したい対象と、検出したくない対象のいろいろです。

// 検出対象
const a = 0 === undefined;
const aa = 0 === null;

const fuga = "null";
const a = fuga === undefined;

const b = "fuga" === undefined;

const button = { text: null };
const c = button.text === null;

const d = "fuga" === null;

const e = accentPhrase.pauseMora === null

// 非検出対象
const COMBINATION_IS_NONE = "####";
const b = 0 === COMBINATION_IS_NONE;

const c = loadedHotkey.action === defaultHotkey.action

@k-chop
Copy link
Contributor

k-chop commented Sep 27, 2023

(横から突然すみません)

ss1
[right.value=undefined]undefined' でくくってないので、rightvalue が存在しないケースにひっかかっているんだと思います。

nullとundefinedのASTはそれぞれこんな感じなので、
nullはtype=literal, value=null、undefinedはtype=Identifier, name=identifier を見てあげるとよさそうです
ss2
ss3

ASTはここで見れます
(デフォルトだとacornだった気がするので、試す場合はparserを typescript-eslint/parser にセットしておくのを忘れずに)
https://astexplorer.net

image

.eslintrc.js Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

Hiroshiba commented Sep 27, 2023

@k-chop おおなるほどです!!!!!詳しくありがとうございます!!!
AST見れるの便利ですね!!!

@Tksn07
Copy link
Contributor Author

Tksn07 commented Sep 27, 2023

@k-chop おぉぉぉぉ!!こんな便利ツールがあるんですね!!!
教えてくださりありがとうございます!!

@Tksn07 Tksn07 requested a review from Hiroshiba September 27, 2023 15:21
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!!

プルリクエストの Files changedの下にワーニングが大量に出ていますが、今しばらくは無視する形で良いのかなと思いました!

まだ禁止する(エラーにする)ところまで到達していないので、元のissueはopenのままにしておこうと思います。

@Tksn07 さん
初のプルリクエストありがとうございました!!(ちなみに問題は全くなかったです!)
もしよかったらまたプルリクエストをいただけるととても嬉しいです!
ワーニングを消していくとか、是非・・・!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants